Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for DSF files #2379

Merged
merged 3 commits into from
Jan 10, 2017
Merged

Support for DSF files #2379

merged 3 commits into from
Jan 10, 2017

Conversation

docbobo
Copy link
Contributor

@docbobo docbobo commented Jan 9, 2017

Adds support for DSF Files to beets, based on the work in quodlibet/mutagen#283. This should resolve #459.

Although this works locally, I am not sure if this is all that needs to be done. So please review and let me know if there will be additional changes required.

Please do not merge before quodlibet/mutagen#283 has been accepted.

@sampsyo
Copy link
Member

sampsyo commented Jan 9, 2017

Awesome! This looks good so far.

Would you mind also adding a test file to test/rsrc and associated boilerplate to test/test_mediafile.py?

Also, one thing we've done in the past for Mutagen dependencies is to use feature detection. That way we can release a beets version with the new support before the code is even merged into Mutagen. That's not necessary, of course—if the Mutagen side is ready soon, we can just bump our version number requirement.

@docbobo
Copy link
Contributor Author

docbobo commented Jan 9, 2017

I did add the requested tests to test/test_mediafile.py, though I am not completely sure that I created the correct test/rsrc/unparseable.dsf - perhaps you have some background on that?

Regarding the feature detection: where would I have to look for more details on how that works?

@sampsyo
Copy link
Member

sampsyo commented Jan 9, 2017

Things look great so far! I'll check out the test now.

For this particular extension, I actually don't know if any code is necessary at all to maintain compatibility. I don't think the import mutagen.dsf line is necessary; Mutagen kindly does the file type detection for us. See b9500c3, where I've removed all the unnecessary Mutagen imports. And without that, I think things should "just work" before and after the support is added to the library.

For the tests, we might do something like:

try:
    import mutagen.dsf
except:
    HAVE_DSF = True
else:
    HAVE_DSF = False

followed by @unittest.skipIf(not HAVE_DSF) on the tests in question.

@@ -77,6 +78,7 @@
'mpc': 'Musepack',
'asf': 'Windows Media',
'aiff': 'AIFF',
'dsf': 'DSD Stream File',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not quite the expansion of the acronym, but maybe "DSD Stream" or even just "DSD" would better match the other short names we have here, none of which have a "file" or "audio" suffix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there is also DSDIFF. Although it doesn't support tags, it might be valuable to be as precise as possible here. But if you want me to change that, I'll happily do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arg, you're right—it's weird that there are competing standards here. But I agree in that case; let's keep the full name.

@sampsyo
Copy link
Member

sampsyo commented Jan 9, 2017

For the unparseable test, it looks like the intent is to include a date field that's empty (instead of committing the date file altogether). Here, for example, are the contents for unparseable.mp3:

>>> mutagen.File('test/rsrc/unparseable.mp3')
{'TDRC': TDRC(encoding=<Encoding.UTF8: 3>, text=[u''])}

It looks like the current test file has no tags at all:

>>> mutagen.File('test/rsrc/unparseable.dsf')
{}

Maybe the test file should just use the same empty-string frame as the MP3?

I realize this is confusing and poorly documented—I'm going to add some comments to the source now to make this more intelligible.

@docbobo
Copy link
Contributor Author

docbobo commented Jan 10, 2017

Ok, I've "fixed" the unparseabl.dsf and also added the DSF feature toggle to the test case. Let me know if there's anything else.

- Fixed unparseable.dsf
- Added DSF feature detection to test_mediafile.py
@docbobo
Copy link
Contributor Author

docbobo commented Jan 10, 2017

And let me know if you want me to rebase everything into one commit before merging the PR...

@sampsyo sampsyo merged commit e0a4dc6 into beetbox:master Jan 10, 2017
sampsyo added a commit that referenced this pull request Jan 10, 2017
sampsyo added a commit that referenced this pull request Jan 10, 2017
sampsyo added a commit that referenced this pull request Jan 10, 2017
There isn't currently an `image.dsf`, so those tests fail.
@sampsyo
Copy link
Member

sampsyo commented Jan 10, 2017

Looks great; thanks! I merged this and did some tests locally, so we should be good to go.

I had to turn off the image tests because there isn't an image.dsf test fixture file yet. We can re-enable those, of course, if you think it would be helpful to add that fixture too.

sampsyo added a commit that referenced this pull request Jan 10, 2017
@docbobo
Copy link
Contributor Author

docbobo commented Jan 10, 2017

I do have the image.dsf here. Do you believe there is value in running those tests? It's pretty much basic ID3.

@sampsyo
Copy link
Member

sampsyo commented Jan 10, 2017

This sounds a bit ambivalent, but I'll leave that up to you—if you think there's anything that might be revealed about the DSF format with images (e.g., when tags get much larger than in the plain-text case), then let's do it. But if you think it's totally redundant with the MP3 tests, there's no need.

@docbobo
Copy link
Contributor Author

docbobo commented Jan 10, 2017

Ok. Let's keep it the way it is then.

@docbobo docbobo deleted the feature/dsf-support branch January 10, 2017 19:11
@sampsyo
Copy link
Member

sampsyo commented Jan 10, 2017

Sounds good! Thank you again for all your work on this. ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DSD/DSF file format support
3 participants